Add serial::Read/Write implementation #9

rnestler · 2018-06-09T15:36:06Z

So this is just a quick PoC to implement #8.
@japaric Do you think it would be OK to implement it in that way? If yes I can finish it up.

posborne · 2018-08-15T04:28:44Z

CC @rust-embedded/embedded-linux

hannobraun · 2018-10-10T13:43:05Z

@posborne You assigned yourself as reviewer a while ago. Have you had a chance to review this PR?

12: Release new Patch on crates.io with InputPin Support r=therealprof a=Caemor Since #9 still seems to be WIP a small update for crates.io would be nice, so I could move back to the crates.io version of this package. Co-authored-by: Chris <caemor@users.noreply.github.com> Co-authored-by: Chris <github@caemor.de>

ryankurte · 2018-12-21T01:54:58Z

Hi hi, sorry this has taken so long, seems like a good addition to me ^_^

@rnestler do you have a public example using this that we could have look at?
@hannobraun @posborne do you have any outstanding concerns?

hannobraun

@rnestler @ryankurte
My comment here was mostly a drive-by comment to figure out why this is stalled. I'm not very familiar with linux-embedded-hal (or embedded Linux in general), but the various unwraps stood out to me (see comments).

src/serial_impl.rs

rnestler · 2018-12-22T16:58:44Z

My comment here was mostly a drive-by comment to figure out why this is stalled. I'm not very familiar with linux-embedded-hal (or embedded Linux in general), but the various unwraps stood out to me (see comments).

Yes, that's what I mean with "this is just a quick PoC" 😉 I wanted some general feedback first before I spend time cleaning it up.

do you have a public example using this that we could have look at?

I think I used it on a Raspberry Pi. I can try to come up with example code that uses it.

posborne

This got buried but this seems good to me so approved from my perspective. I would like a review from the HAL team as I'm not quite as up to date on the current discussions around the traits.

src/serial_impl.rs

dbrgn

~~The serial crate has been split up, we should depend on serial_core instead. You can apply this commit (git apply patch.diff): https://gist.github.com/dbrgn/3f3509acbb8cc9fdde229138858e5d0d~~

(Edit: Disregard what I wrote, the patch is wrong because SerialPort is a trait object. However, we can use serial_unix instead of serial, which simplifies a few things. I'll create a new patch soon.)

src/lib.rs

src/serial_impl.rs

dbrgn · 2019-05-19T21:46:37Z

@rnestler I continued with your branch (after rebasing it against master), see commits here: https://github.com/dbrgn/linux-embedded-hal/commits/implement-serial Feel free to pull them into your branch.

The overall implementation seems to work fine, I'm using it successfully here.

Regarding the tests, I finished the openpty based implementation, but I lack the knowledge of how PTYs work to make the read call return anything. Right now I can write to the pseudo-terminal, but reading results in a timeout.

Would be great if we could get this merged soon after resolving that blocker.

rnestler · 2019-05-23T15:16:28Z

@dbrgn I integrated your commits and implemented some proper tests using the openpty crate. This lead to me discovering that the errors weren't mapped properly.
I may need to rebase and clean up the history a bit if this is important to the maintainers.

dbrgn · 2019-06-06T08:58:54Z

@rnestler what's the status on this?

rnestler · 2019-06-06T16:05:27Z

Ready for review on my side.

ryankurte

lgtm! thanks for all your effort ^_^

ryankurte · 2019-06-08T07:19:35Z

bors r+

bors · 2019-06-08T07:19:36Z

👎 Rejected by code reviews

ryankurte · 2019-06-08T07:47:19Z

whoops, boop @dbrgn

src/serial.rs

By not using the platform-independent implementation, we can simplify some things.

This allows a user to directly use Serial::open without needing to now about serial_unix::TTYPort.

dbrgn

Looks good to me! 👍 Thanks.

posborne · 2019-06-10T19:04:17Z

LGTM. bors r+

rnestler · 2019-06-12T11:44:34Z

Hmm bors doesn't seem to approve...

rnestler · 2019-06-15T13:43:32Z

@posborne What is the status on this?

ryankurte · 2019-06-16T02:16:04Z

ahh, iirc bors commands have to be on their own line?

bors r+

@japaric

9: Add serial::Read/Write implementation r=ryankurte a=rnestler So this is just a quick PoC to implement #8. @japaric Do you think it would be OK to implement it in that way? If yes I can finish it up. Co-authored-by: Raphael Nestler <raphael.nestler@gmail.com> Co-authored-by: Raphael Nestler <raphael.nestler@sensirion.com> Co-authored-by: Danilo Bargen <mail@dbrgn.ch>

bors · 2019-06-16T02:19:30Z

Build succeeded

continuous-integration/travis-ci/push

dbrgn · 2019-06-16T18:50:33Z

Thanks!

rnestler changed the title ~~WIP: Add serial::Read implementation~~ WIP: Add serial::Read/Write implementation Jun 13, 2018

posborne self-requested a review August 15, 2018 04:29

hannobraun added the S-waiting-on-review Status: Review is incomplete label Oct 10, 2018

caemor mentioned this pull request Oct 25, 2018

Release new Patch on crates.io with InputPin Support #12

Merged

hannobraun reviewed Dec 21, 2018

View reviewed changes

src/serial_impl.rs Outdated Show resolved Hide resolved

src/serial_impl.rs Outdated Show resolved Hide resolved

src/serial_impl.rs Outdated Show resolved Hide resolved

rnestler force-pushed the implement-serial branch from 50d5b82 to deccbc1 Compare December 24, 2018 14:06

rnestler requested a review from a team as a code owner December 24, 2018 14:06

posborne previously approved these changes Feb 11, 2019

View reviewed changes

ryankurte requested changes Feb 12, 2019

View reviewed changes

src/serial_impl.rs Outdated Show resolved Hide resolved

rnestler dismissed posborne’s stale review via 176fc28 February 14, 2019 15:22

mathk added S-waiting-on-author Status: Author needs to make changes and removed S-waiting-on-review Status: Review is incomplete labels Feb 22, 2019

dbrgn suggested changes May 14, 2019

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/serial_impl.rs Outdated Show resolved Hide resolved

rnestler force-pushed the implement-serial branch from 176fc28 to 77b8d2c Compare May 21, 2019 13:44

dbrgn mentioned this pull request May 22, 2019

Add RN2xx3 WIP driver rust-embedded/awesome-embedded-rust#186

Merged

rnestler force-pushed the implement-serial branch 2 times, most recently from 96e8151 to ba9b608 Compare June 6, 2019 16:05

rnestler changed the title ~~WIP: Add serial::Read/Write implementation~~ Add serial::Read/Write implementation Jun 6, 2019

ryankurte previously approved these changes Jun 8, 2019

View reviewed changes

ryankurte reviewed Jun 8, 2019

View reviewed changes

src/serial.rs Show resolved Hide resolved

rnestler dismissed ryankurte’s stale review via d7e4cb7 June 8, 2019 14:30

rnestler requested a review from a team as a code owner June 8, 2019 14:30

rnestler force-pushed the implement-serial branch from d7e4cb7 to 7e5a1bd Compare June 8, 2019 14:48

rnestler and others added 7 commits June 8, 2019 17:02

Add serial::Read implementation

281ebd3

Add serial::Write implementation

afc8be6

Use openpty for tests

1128b80

Switch from serial to serial_unix

fe13ba7

By not using the platform-independent implementation, we can simplify some things.

Don't expose serial module directoy

ba723e1

Add tests and implement correct io::Error mapping

1fe6b63

Wrap TTYPort::open

b4239f1

This allows a user to directly use Serial::open without needing to now about serial_unix::TTYPort.

rnestler force-pushed the implement-serial branch from 7e5a1bd to b4239f1 Compare June 8, 2019 15:03

dbrgn approved these changes Jun 10, 2019

View reviewed changes

posborne approved these changes Jun 10, 2019

View reviewed changes

bors bot merged commit b4239f1 into rust-embedded:master Jun 16, 2019

rnestler deleted the implement-serial branch June 16, 2019 08:14

rnestler mentioned this pull request Jun 20, 2019

Feature request: Add Serial implementation #8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add serial::Read/Write implementation #9

Add serial::Read/Write implementation #9

rnestler commented Jun 9, 2018

posborne commented Aug 15, 2018

hannobraun commented Oct 10, 2018

ryankurte commented Dec 21, 2018

hannobraun left a comment

rnestler commented Dec 22, 2018

posborne left a comment

dbrgn left a comment •

edited

dbrgn commented May 19, 2019

rnestler commented May 23, 2019

dbrgn commented Jun 6, 2019

rnestler commented Jun 6, 2019

ryankurte left a comment

ryankurte commented Jun 8, 2019

bors bot commented Jun 8, 2019

ryankurte commented Jun 8, 2019

dbrgn left a comment

posborne commented Jun 10, 2019

rnestler commented Jun 12, 2019

rnestler commented Jun 15, 2019

ryankurte commented Jun 16, 2019

bors bot commented Jun 16, 2019

dbrgn commented Jun 16, 2019

Add serial::Read/Write implementation #9

Add serial::Read/Write implementation #9

Conversation

rnestler commented Jun 9, 2018

posborne commented Aug 15, 2018

hannobraun commented Oct 10, 2018

ryankurte commented Dec 21, 2018

hannobraun left a comment

Choose a reason for hiding this comment

rnestler commented Dec 22, 2018

posborne left a comment

Choose a reason for hiding this comment

dbrgn left a comment • edited

Choose a reason for hiding this comment

dbrgn commented May 19, 2019

rnestler commented May 23, 2019

dbrgn commented Jun 6, 2019

rnestler commented Jun 6, 2019

ryankurte left a comment

Choose a reason for hiding this comment

ryankurte commented Jun 8, 2019

bors bot commented Jun 8, 2019

ryankurte commented Jun 8, 2019

dbrgn left a comment

Choose a reason for hiding this comment

posborne commented Jun 10, 2019

rnestler commented Jun 12, 2019

rnestler commented Jun 15, 2019

ryankurte commented Jun 16, 2019

bors bot commented Jun 16, 2019

Build succeeded

dbrgn commented Jun 16, 2019

dbrgn left a comment •

edited